Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Resource: aws_ecs_cluster_capacity_providers #22672

Merged
merged 8 commits into from
Jan 26, 2022
Merged

New Resource: aws_ecs_cluster_capacity_providers #22672

merged 8 commits into from
Jan 26, 2022

Conversation

roberth-k
Copy link
Contributor

@roberth-k roberth-k commented Jan 20, 2022 •
edited
Loading

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Relates #5278
Relates #11351
Closes #11409
Closes #11531
Relates #22754

This is based on the S3 resource split, as it's a similar case of PUT-s modifying specific aspects of a resource.

In a similar vein it might make sense to deprecate the capacity_providers and default_capacity_provider_strategy of aws_ecs_cluster in 4.0, but I'm not sure whether it's too late for that. In any case, this would be achieved in a separate PR.

Output from acceptance testing:

$ make testacc PKG=ecs TESTS=TestAccECSClusterCapacityProviders_ && make testacc PKG=ecs TESTS=TestAccECSCluster_ && make testacc PKG=ecs TESTS=TestAccECSClusterDataSource_
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSClusterCapacityProviders_'  -timeout 180m
=== RUN   TestAccECSClusterCapacityProviders_basic
=== PAUSE TestAccECSClusterCapacityProviders_basic
=== RUN   TestAccECSClusterCapacityProviders_disappears
=== PAUSE TestAccECSClusterCapacityProviders_disappears
=== RUN   TestAccECSClusterCapacityProviders_defaults
=== PAUSE TestAccECSClusterCapacityProviders_defaults
=== RUN   TestAccECSClusterCapacityProviders_destroy
=== PAUSE TestAccECSClusterCapacityProviders_destroy
=== RUN   TestAccECSClusterCapacityProviders_Update_capacityProviders
=== PAUSE TestAccECSClusterCapacityProviders_Update_capacityProviders
=== RUN   TestAccECSClusterCapacityProviders_Update_defaultStrategy
=== PAUSE TestAccECSClusterCapacityProviders_Update_defaultStrategy
=== CONT  TestAccECSClusterCapacityProviders_basic
=== CONT  TestAccECSClusterCapacityProviders_destroy
=== CONT  TestAccECSClusterCapacityProviders_Update_defaultStrategy
=== CONT  TestAccECSClusterCapacityProviders_disappears
=== CONT  TestAccECSClusterCapacityProviders_defaults
=== CONT  TestAccECSClusterCapacityProviders_Update_capacityProviders
--- PASS: TestAccECSClusterCapacityProviders_disappears (45.38s)
--- PASS: TestAccECSClusterCapacityProviders_defaults (46.08s)
--- PASS: TestAccECSClusterCapacityProviders_basic (46.40s)
--- PASS: TestAccECSClusterCapacityProviders_Update_defaultStrategy (109.84s)
--- PASS: TestAccECSClusterCapacityProviders_Update_capacityProviders (109.92s)
--- PASS: TestAccECSClusterCapacityProviders_destroy (264.48s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ecs        266.415s
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSCluster_'  -timeout 180m
=== RUN   TestAccECSCluster_basic
=== PAUSE TestAccECSCluster_basic
=== RUN   TestAccECSCluster_disappears
=== PAUSE TestAccECSCluster_disappears
=== RUN   TestAccECSCluster_tags
=== PAUSE TestAccECSCluster_tags
=== RUN   TestAccECSCluster_singleCapacityProvider
=== PAUSE TestAccECSCluster_singleCapacityProvider
=== RUN   TestAccECSCluster_capacityProviders
=== PAUSE TestAccECSCluster_capacityProviders
=== RUN   TestAccECSCluster_capacityProvidersUpdate
=== PAUSE TestAccECSCluster_capacityProvidersUpdate
=== RUN   TestAccECSCluster_capacityProvidersNoStrategy
=== PAUSE TestAccECSCluster_capacityProvidersNoStrategy
=== RUN   TestAccECSCluster_containerInsights
=== PAUSE TestAccECSCluster_containerInsights
=== RUN   TestAccECSCluster_configuration
=== PAUSE TestAccECSCluster_configuration
=== CONT  TestAccECSCluster_basic
=== CONT  TestAccECSCluster_capacityProvidersUpdate
=== CONT  TestAccECSCluster_capacityProvidersNoStrategy
=== CONT  TestAccECSCluster_containerInsights
=== CONT  TestAccECSCluster_configuration
=== CONT  TestAccECSCluster_singleCapacityProvider
=== CONT  TestAccECSCluster_capacityProviders
=== CONT  TestAccECSCluster_tags
=== CONT  TestAccECSCluster_disappears
--- PASS: TestAccECSCluster_disappears (23.83s)
--- PASS: TestAccECSCluster_basic (26.14s)
--- PASS: TestAccECSCluster_tags (45.99s)
--- PASS: TestAccECSCluster_capacityProviders (47.35s)
--- PASS: TestAccECSCluster_configuration (48.29s)
--- PASS: TestAccECSCluster_capacityProvidersNoStrategy (54.85s)
--- PASS: TestAccECSCluster_containerInsights (63.75s)
--- PASS: TestAccECSCluster_singleCapacityProvider (77.99s)
--- PASS: TestAccECSCluster_capacityProvidersUpdate (90.17s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ecs        92.042s
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSClusterDataSource_'  -timeout 180m
=== RUN   TestAccECSClusterDataSource_ecsCluster
=== PAUSE TestAccECSClusterDataSource_ecsCluster
=== RUN   TestAccECSClusterDataSource_ecsClusterContainerInsights
=== PAUSE TestAccECSClusterDataSource_ecsClusterContainerInsights
=== CONT  TestAccECSClusterDataSource_ecsCluster
=== CONT  TestAccECSClusterDataSource_ecsClusterContainerInsights
--- PASS: TestAccECSClusterDataSource_ecsCluster (22.93s)
--- PASS: TestAccECSClusterDataSource_ecsClusterContainerInsights (22.93s)
PASS
ok      github.com/hashicorp/terraform-provider-aws/internal/service/ecs        24.876s

@github-actions github-actions bot added documentation Introduces or discusses updates to documentation. provider Pertains to the provider itself, rather than any interaction with AWS. service/ecs Issues and PRs that pertain to the ecs service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. size/XL Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. labels Jan 20, 2022
@roberth-k roberth-k force-pushed the f-aws_ecs_cluster_capacity_providers branch from bfe717d to 5a52b93 Compare January 20, 2022 01:17
@roberth-k roberth-k force-pushed the f-aws_ecs_cluster_capacity_providers branch from 5a52b93 to d5166e7 Compare January 20, 2022 10:00
@justinretzolk justinretzolk added new-resource Introduces a new resource. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 20, 2022
@YakDriver
Copy link
Member

YakDriver commented Jan 24, 2022 •
edited
Loading

@roberth-k You list this PR as related to #11409. However, you could make the argument that your PR fixes #11409, through use of this resource to make the association, and then avoiding the dependency block on delete. You can still run into the problem in the same way, using the capacity_providers arguments of aws_ecs_cluster, so you could also make the argument that it does not fix #11409. Perhaps that is what you were thinking.

@YakDriver YakDriver force-pushed the f-aws_ecs_cluster_capacity_providers branch from 9b7948f to 1812535 Compare January 24, 2022 22:38
@YakDriver YakDriver self-assigned this Jan 24, 2022
@roberth-k
Copy link
Contributor Author

@YakDriver In principle, the PR closes the headline of #11409. I chose to mark it as Relates as it doesn't resolve #5278, nor does it make a clear decision about whether the capacity provider attributes of aws_ecs_cluster should be deprecated for 4.0 similar to S3 (both mentioned in #11409 (comment)).

I was planning to approach the remaining questions of #11409 separately once this PR is merged.

@YakDriver
Copy link
Member

YakDriver commented Jan 25, 2022 •
edited
Loading

Since the vast majority of upvotes for #11409 are likely from the error whilst destroying clusters and this PR provides the recommended way around that, let's have this PR close #11409. I recognize that a complete solution includes 1) resolving #5278, and 2) deprecating the capacity provider arguments (#22754). However, #5278 is already an issue and I've created an issue for deprecation (#22754).

@YakDriver YakDriver added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 25, 2022
YakDriver
YakDriver requested changes Jan 25, 2022
View reviewed changes
Copy link
Member

@YakDriver YakDriver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few minor changes but looking very good:

Acceptance test output:

% make testacc TESTS=TestAccECSCluster PKG=ecs
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSCluster'  -timeout 180m
--- PASS: TestAccECSCluster_disappears (26.60s)
--- PASS: TestAccECSClusterDataSource_ecsCluster (29.15s)
--- PASS: TestAccECSClusterDataSource_ecsClusterContainerInsights (29.15s)
--- PASS: TestAccECSCluster_basic (29.60s)
--- PASS: TestAccECSCluster_tags (49.86s)
--- PASS: TestAccECSClusterCapacityProviders_disappears (52.17s)
--- PASS: TestAccECSClusterCapacityProviders_basic (52.19s)
--- PASS: TestAccECSClusterCapacityProviders_defaults (52.28s)
--- PASS: TestAccECSCluster_capacityProviders (52.54s)
--- PASS: TestAccECSCluster_capacityProvidersNoStrategy (55.50s)
--- PASS: TestAccECSCluster_configuration (56.24s)
--- PASS: TestAccECSCluster_containerInsights (68.45s)
--- PASS: TestAccECSCluster_capacityProvidersUpdate (84.96s)
--- PASS: TestAccECSCluster_singleCapacityProvider (106.63s)
--- PASS: TestAccECSClusterCapacityProviders_update_defaultCapacityProviderStrategy (119.66s)
--- PASS: TestAccECSClusterCapacityProviders_update_capacityProviders (119.88s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ecs	121.190s

internal/provider/provider.go Show resolved Hide resolved
internal/service/ecs/cluster.go Outdated Show resolved Hide resolved
internal/service/ecs/cluster.go Outdated Show resolved Hide resolved
internal/service/ecs/cluster_capacity_providers.go Show resolved Hide resolved
internal/service/ecs/cluster_capacity_providers.go Show resolved Hide resolved
internal/service/ecs/cluster_capacity_providers_test.go Outdated Show resolved Hide resolved
internal/service/ecs/cluster_capacity_providers_test.go Show resolved Hide resolved
internal/service/ecs/cluster_test.go Show resolved Hide resolved
website/docs/r/ecs_cluster.html.markdown Outdated Show resolved Hide resolved
website/docs/r/ecs_cluster_capacity_providers.html.markdown Outdated Show resolved Hide resolved
@YakDriver YakDriver added this to the Roadmap milestone Jan 25, 2022
This was referenced Jan 25, 2022
Closed
Closed
@roberth-k roberth-k force-pushed the f-aws_ecs_cluster_capacity_providers branch from d3f73c5 to f4c7937 Compare January 26, 2022 16:35
@roberth-k roberth-k requested a review from YakDriver January 26, 2022 17:17
YakDriver
YakDriver approved these changes Jan 26, 2022
View reviewed changes
Copy link
Member

@YakDriver YakDriver left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🎉

Output from acceptance tests (us-west-2):

% make testacc TESTS=TestAccECSCluster PKG=ecs
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSCluster'  -timeout 180m
--- PASS: TestAccECSCluster_disappears (32.18s)
--- PASS: TestAccECSClusterDataSource_ecsCluster (36.98s)
--- PASS: TestAccECSClusterDataSource_ecsClusterContainerInsights (37.44s)
--- PASS: TestAccECSCluster_basic (40.06s)
--- PASS: TestAccECSClusterCapacityProviders_disappears (58.45s)
--- PASS: TestAccECSClusterCapacityProviders_defaults (63.82s)
--- PASS: TestAccECSClusterCapacityProviders_basic (63.85s)
--- PASS: TestAccECSCluster_capacityProvidersNoStrategy (66.59s)
--- PASS: TestAccECSCluster_capacityProviders (67.29s)
--- PASS: TestAccECSCluster_tags (69.36s)
--- PASS: TestAccECSCluster_configuration (73.15s)
--- PASS: TestAccECSCluster_containerInsights (84.24s)
--- PASS: TestAccECSCluster_capacityProvidersUpdate (91.86s)
--- PASS: TestAccECSCluster_singleCapacityProvider (103.78s)
--- PASS: TestAccECSClusterCapacityProviders_Update_capacityProviders (139.93s)
--- PASS: TestAccECSClusterCapacityProviders_Update_defaultStrategy (140.02s)
--- PASS: TestAccECSClusterCapacityProviders_destroy (237.35s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ecs	238.879s

GovCloud:

% make testacc TESTS=TestAccECSCluster PKG=ecs
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/ecs/... -v -count 1 -parallel 20 -run='TestAccECSCluster'  -timeout 180m
--- PASS: TestAccECSCluster_disappears (39.75s)
--- PASS: TestAccECSClusterDataSource_ecsClusterContainerInsights (44.05s)
--- PASS: TestAccECSClusterDataSource_ecsCluster (44.06s)
--- PASS: TestAccECSCluster_basic (48.29s)
--- PASS: TestAccECSClusterCapacityProviders_disappears (58.49s)
--- PASS: TestAccECSClusterCapacityProviders_defaults (70.46s)
--- PASS: TestAccECSClusterCapacityProviders_basic (70.53s)
--- PASS: TestAccECSCluster_capacityProviders (76.30s)
--- PASS: TestAccECSCluster_configuration (81.95s)
--- PASS: TestAccECSCluster_tags (84.44s)
--- PASS: TestAccECSCluster_singleCapacityProvider (92.50s)
--- PASS: TestAccECSCluster_capacityProvidersNoStrategy (93.20s)
--- PASS: TestAccECSCluster_containerInsights (98.42s)
--- PASS: TestAccECSCluster_capacityProvidersUpdate (105.06s)
--- PASS: TestAccECSClusterCapacityProviders_Update_capacityProviders (159.09s)
--- PASS: TestAccECSClusterCapacityProviders_Update_defaultStrategy (159.12s)
--- PASS: TestAccECSClusterCapacityProviders_destroy (229.52s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/ecs	231.487s

@YakDriver YakDriver modified the milestones: Roadmap, v3.74.0 Jan 26, 2022
@YakDriver YakDriver merged commit c47375e into hashicorp:main Jan 26, 2022
@github-actions github-actions bot removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 26, 2022
@ewbankkit ewbankkit mentioned this pull request Jan 27, 2022
Merged
@github-actions
Copy link

This functionality has been released in v3.74.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@roberth-k roberth-k deleted the f-aws_ecs_cluster_capacity_providers branch February 10, 2022 20:39
@stewartcampbell stewartcampbell mentioned this pull request Feb 22, 2022
Merged
This was referenced Apr 3, 2022
Closed
Closed
Closed
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@YakDriver YakDriver YakDriver approved these changes

Assignees

@YakDriver YakDriver

Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/ecs Issues and PRs that pertain to the ecs service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v3.74.0
Development

Successfully merging this pull request may close these issues.

aws_ecs_capacity_provider_attachment aws_ecs_cluster with capacity_providers cannot be destroyed
3 participants
@roberth-k @YakDriver @justinretzolk